Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Rework PID class API #246

Open
wants to merge 35 commits into
base: ros2-master
Choose a base branch
from
Open

Rework PID class API #246

wants to merge 35 commits into from
+774 −231

Conversation

christophfroehlich
Copy link
Contributor

@christophfroehlich christophfroehlich commented Dec 7, 2024 •
edited
Loading

While fixing the clang errors I realized that lots of implicit casts to computeCommand were used, and the typical usage was to pass something like period.nanoseconds() and convert it internally to seconds. Because introducing an overload with double dt as argument is ambiguous and might silently break user code, I decided to convert the class methods from camelCase to snake_case (the ros2_control standard), remove the uint64_t version but introduce the following new overloads which converts dt to seconds in a consistent way

  • double dt
  • rcl_duration_value_t dt_ns
  • rclcpp::Duration dt
  • std::chrono::nanoseconds dt_ns

I converted the method names of PidROS accordingly as well, but left the only rclcpp::Duration version there.

The old methods remain deprecated, this should not break anything in downstream packages.
AFAIK adding non-virtual methods should not be a problem regarding ABI break?

@codecov-commenter
Copy link

codecov-commenter commented Dec 7, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 92.85714% with 13 lines in your changes missing coverage. Please review.

Project coverage is 75.93%. Comparing base (6611872) to head (188f3be).

Files with missing lines Patch % Lines
src/pid_ros.cpp 80.35% 11 Missing ⚠️
src/pid.cpp 93.75% 2 Missing ⚠️
Additional details and impacted files 
@@               Coverage Diff               @@
##           ros2-master     #246      +/-   ##
===============================================
+ Coverage        74.88%   75.93%   +1.04%     
===============================================
  Files               24       24              
  Lines             1107     1155      +48     
  Branches            86       88       +2     
===============================================
+ Hits               829      877      +48     
  Misses             231      231              
  Partials            47       47
Flag Coverage Δ
unittests 75.93% <92.85%> (+1.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
include/control_toolbox/pid.hpp 86.66% <100.00%> (+8.88%) ⬆️
include/control_toolbox/pid_ros.hpp 100.00% <ø> (ø)
test/pid_ros_parameters_tests.cpp 100.00% <100.00%> (ø)
test/pid_ros_publisher_tests.cpp 95.23% <100.00%> (ø)
test/pid_tests.cpp 100.00% <100.00%> (ø)
src/pid.cpp 93.67% <93.75%> (+1.02%) ⬆️
src/pid_ros.cpp 72.41% <80.35%> (ø)

@mergify Mergify
Copy link

mergify bot commented Dec 7, 2024

This pull request is in conflict. Could you fix it @christophfroehlich?

@christophfroehlich christophfroehlich changed the title Add computeCommand with double dt_s argument Rework PID class API Dec 13, 2024
saikishor
saikishor reviewed Dec 13, 2024
View reviewed changes
include/control_toolbox/pid.hpp Outdated Show resolved Hide resolved
include/control_toolbox/pid.hpp Outdated Show resolved Hide resolved
include/control_toolbox/pid.hpp Outdated Show resolved Hide resolved
include/control_toolbox/pid_ros.hpp Outdated Show resolved Hide resolved
include/control_toolbox/pid_ros.hpp Outdated Show resolved Hide resolved
include/control_toolbox/pid_ros.hpp Outdated Show resolved Hide resolved
christophfroehlich
christophfroehlich commented Dec 13, 2024
View reviewed changes
src/pid.cpp Outdated Show resolved Hide resolved
christophfroehlich
christophfroehlich commented Dec 13, 2024
View reviewed changes
src/pid_ros.cpp Outdated Show resolved Hide resolved
christophfroehlich
christophfroehlich commented Dec 13, 2024
View reviewed changes
src/pid_ros.cpp Outdated Show resolved Hide resolved
christophfroehlich
christophfroehlich commented Dec 13, 2024
View reviewed changes
src/pid_ros.cpp Outdated Show resolved Hide resolved
christophfroehlich
christophfroehlich commented Dec 13, 2024
View reviewed changes
src/pid_ros.cpp Outdated Show resolved Hide resolved
@christophfroehlich christophfroehlich mentioned this pull request Jan 21, 2025
Open
ViktorCVS
ViktorCVS approved these changes Jan 22, 2025
View reviewed changes
Copy link

@ViktorCVS ViktorCVS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! All variable and function names have been appropriately replaced in every instance where they appear.

@mergify Mergify
Copy link

mergify bot commented Jan 23, 2025

This pull request is in conflict. Could you fix it @christophfroehlich?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ViktorCVS ViktorCVS ViktorCVS approved these changes

@aprotyas aprotyas Awaiting requested review from aprotyas

@ARK3r ARK3r Awaiting requested review from ARK3r

@destogl destogl Awaiting requested review from destogl

@erickisos erickisos Awaiting requested review from erickisos

@harderthan harderthan Awaiting requested review from harderthan

@olivier-stasse olivier-stasse Awaiting requested review from olivier-stasse

@saikishor saikishor Awaiting requested review from saikishor

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@christophfroehlich @codecov-commenter @saikishor @ViktorCVS